Repository navigation
fix(parser): derive the nullable-boolean spelling set (2.5.2) - #197
Merged
Merged
Conversation
2.5.1 hand-wrote _NPM_NULLABLE_BOOLEAN_FLAGS with 12 entries. npm's five nullable boolean definitions have 18 spellings -- `y`, `ws`, `n` and `no` are shorthands (`n` and `no` both expand to `--no-yes`) and each is emitted in both `-x` and `--x` form -- so `--y -ws -n --n -no --no` each read a literal `null` as the PACKAGE rather than as the flag's value. `null` is a real published npm package, so that mints a wrong identity, which refresher.py's freshness gate then treats as positive confirmation. derive_npm_flags.py now derives the table: a spelling is nullable iff its resolution target, after shorthand expansion and after stripping a leading `no-`, is a definition whose declared type includes `null`. `--verify` gains a `null` probe per definition AND a spelling-level probe over all 442 enumerable spellings, plus a nullable-subset-of-boolean check. Confirmed the new probe rejects the shipped 2.5.1 set with exactly 6 mismatches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3
…alue test Parametrised pins for every spelling that consumes a literal `null` and for a sample that must NOT, plus the identity consequence (`-n null server-a` and `-n null server-b` stayed distinct) and a nullable-subset-of-boolean check that holds without npm present. test_baked_value_shorthand_still_skips_an_ordinary_token was absorbed into TestOnlyNullableBooleansConsumeNull; it tests baked-value shorthands, so it goes back to TestNpmBakedValueShorthandDoesNotConsume. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3
Patch hotfix for the six npm flag spellings 2.5.1 regressed. Notes in CHANGELOG.md that the wider parser gap in #195 stays open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3
… exact set Board review, adversarial seat, on the 2.5.2 diff. The CHANGELOG claimed 2.5.1 regressed six spellings that 2.5.0 handled correctly. Verified false by running each released version's own detect_package_type: v2.4.1, v2.5.0 and v2.5.1 all resolve 'npm exec -n null zz' to the package 'null'. The blanket rule that handled them correctly lived on main between two unreleased commits only, so no shipped version was ever right. 2.5.2 is the first. Also pins the nullable table by VALUE. Every other check was one-directional -- the parametrized spellings prove each of the 18 consumes null, the subset check proves none is unreachable, and an EXTRA entry passed both. An extra entry is a wrong identity in the other direction. Verified the new pin goes red when --global is injected into the table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The regression
#194 introduced
_NPM_NULLABLE_BOOLEAN_FLAGSto decide whether a literalnullafter an npm boolean flag is the flag's value or the package name.nullis a real published npm package, so getting it wrong mints a wrong package identity — andrefresher.py's freshness gate treats a matching identity as positive confirmation that a cached tool description still describes the configured package. Two different servers can therefore be served each other's tool descriptions.That table was written by hand, with 12 entries. npm has five nullable boolean definitions (
yes,optional,production,workspaces,expect-results) but eighteen spellings for them:y,ws,nandnoare shorthands —nandnoboth expand to--no-yes— and every short alias / shorthand key is legal in both its-xand--xform.Six spellings were missing. Shipped in 2.5.1:
-yzzzzzzzz--yeszzzzzzzz--yzznullzzzz-wszznullzzzz-nzznullzzzz--nzznullzzzz-nozznullzzzz--nozznullzzzz(input
npm exec <flag> null zz). 2.5.0 got all six right, so this is strictly a regression from #194.The fix: derive it
Four consecutive defects in this parser trace back to hand-transcribing npm's behaviour, so the set is generated, not pasted.
.consiliency/notes/derive_npm_flags.pynow emits a fourth table using this rule:The "raw" matters:
classify()dropsnullfrom the type set (that drop is what makes--yesa boolean at all rather than conditional), so nullability is read before the drop.The generated set, exactly:
Diff against the committed tables:
_NPM_NULLABLE_BOOLEAN_FLAGS+6 / −0;_NPM_VALUE_FLAGS(118),_NPM_BOOLEAN_FLAGS(198) and_NPM_SKIP_FLAGS(16) are byte-identical to what is already committed.--verifynow covers itBefore this PR
--verifychecked three tables and said nothing about the nullable one — zero drift protection. Added:nullprobe per definition, with the prediction "a BOOLEAN consumesnulliff its declared type carriesnull".npm exec <flag> null zz. This is the one that matters — the defect was a spelling omission with a correct definition set, so a definition-level probe alone would have missed it._NPM_NULLABLE_BOOLEAN_FLAGS ⊆ _NPM_BOOLEAN_FLAGS(it is exempt from the pairwise-disjointness check, being a legitimate subset, so it gets a containment check in its place).record()raises on conflicting nullability, not just conflicting class.-y/--yare written twice (fromyes'sshortand from theyshorthand); they agree today, which is not a reason to let a future npm disagree silently.Negative control — the new spelling probe run against the shipped 2.5.1 set returns exactly the six regressed spellings:
Verification
nopt cross-check, every boolean spelling (ad-hoc script,
version_checker.__file__printed to prove it exercised the worktree copy):uv run python .consiliency/notes/derive_npm_flags.py --verify:Full suite:
uv run pytest -q→ 2835 passed, 3 skipped, 25 deselected in 5m33s, exit 0.Reported honestly: the first full run came back
1 failed, 2834 passed, 3 skipped, 1 error, and I do not want that laundered out of the record. Two distinct causes, neither in the diff:ERROR …::test_true_and_false_are_consumed_by_any_boolean— self-inflicted. I ranuv lock,ruff,mypyand a targetedpytestwhile that suite was in flight, and eachuv runre-syncs.venv, swapping the installedpmcpfrom 2.5.1 to 2.5.2 underneath the running interpreter.tests/test_version_checker.pypasses 247/247 in isolation.FAILED tests/runtime/test_subscriptions_e2e.py::test_connect_disconnect_refresh_each_deliver_all_three_kinds— shared host state. That harness asserts the live gateway on:3344gains no child process across the test window; a gateway is running on this host and picked up one new child (3679740) from unrelated activity.uvdoes not spawn children of that process, so this is not the resync — it is the shared gateway.The re-run above ran with nothing else touching the venv and is green.
Named regression classes still green (62 passed):
TestNpmValueFlagCollisions(#180),TestNpmFailsClosed(#180),TestNpmPinnedOrderingSurvives,TestNpmBakedValueShorthandDoesNotConsume(#194),TestNpxBooleanLiterals(#194 npx-literal refusal),TestOnlyNullableBooleansConsumeNull.Manifest: all 98 manifest servers that declare a launch command still resolve — 79 npm + 19 pypi, 0 unknown. (
manifest.yamlhas 107 entries: 3 declare nocommandkey at all, and 6 remote servers declarecommand: "".)ruff check src/ tests/,ruff format --check src/ tests/,mypy src/— all clean. (ruff format --check .reports 4 files, all pre-existing onmainand untouched here:.consiliency/notes/mutate.py,.consiliency/notes/verify_tables.py,diagnostics/issue-79-1b/*.py. CI scopes tosrc/ tests/.)All Python runs used
PYTHONDONTWRITEBYTECODE=1withsrc/**/__pycache__purged first.Not verified here: CI itself, and the install-smoke path — neither was run locally.
Tests
TestOnlyNullableBooleansConsumeNullgains a parametrised pin over all 18 spellings, a counter-pin that-g --global -f --force --no-globalleavenullas the package, the identity consequence stated directly (-n null server-aand-n null server-bstay distinct), and a subset check that holds on a host without npm.Also moved
test_baked_value_shorthand_still_skips_an_ordinary_tokenback toTestNpmBakedValueShorthandDoesNotConsume— it had been absorbed into the nullable class but tests baked-value shorthands.Scope
This does not close #195. Attached values (
--global=pkg), npx's own-p=rewriting, and npx's-nremoval remain unhandled; #195 stays open.Fixes a regression from #194. Relates to #195. Version bumped to 2.5.2 (
pyproject.toml,src/pmcp/__init__.py,uv.lock) with a CHANGELOG entry. No tag created or pushed.🤖 Generated with Claude Code
https://claude.ai/code/session_01T61zMNiPF4K29186r3uqc3